sys/checksum: Adding three new crc16 variations #18516
Conversation
github-actions
bot
added
Area: sys
This might lead to confusion for users that use the old |
miri64
added
Type: new feature
Teufelchen1
force-pushed
the
feat/crc_fixup
branch
from
34efda8 to
9985802
Compare
sys/include/checksum/crc16_ccitt.h
Outdated
| @@ -39,6 +36,8 @@ extern "C" { | |||
|
|
|||
| /** | |||
| * @brief Update CRC16-CCITT | |||
| * @deprecated Use @ref crc16_ccitt_false_update instead. | |||
| * Will be removed after 2023.01 release. | |||
There was a problem hiding this comment.
This might lead to confusion for users that use the old ccitt. Maybe, it is best to move this through proper API aliasing and deprecation, i.e. introduce new crc16_ccitt_() inline functions that wrap around crc16_ccitt_aug_ and add a @Deprecation note that this function will be removed in 2 release cycles (i.e. 2023.01 if this PR gets merged soon). That node could also include a mention that crc16_ccitt_aug_* should be used instead.
@miri64 like this? :)
There was a problem hiding this comment.
This might lead to confusion for users that use the old ccitt. Maybe, it is best to move this through proper API aliasing and deprecation, i.e. introduce new crc16_ccitt__() inline functions that wrap around crc16_ccitt_aug__ and add a @Deprecation note that this function will be removed in 2 release cycles (i.e. 2023.01 if this PR gets merged soon). That node could also include a mention that crc16_ccitt_aug_* should be used instead.
@miri64 like this? :)
Yes, but you also could make this function also now a static inline ... around crc16_ccitt_false_update() now to make the pending deprecation clearer.
Other than that, some formatting issues remain.
Teufelchen1
force-pushed
the
feat/crc_fixup
branch
from
9985802 to
7662eca
Compare
Teufelchen1
force-pushed
the
feat/crc_fixup
branch
from
7662eca to
d762999
Compare
sys/include/checksum/crc16_ccitt.h
Outdated
| * @details polynom=0x1021 | ||
| * seed=0x1d0f | ||
| * check=0xe5cc |
There was a problem hiding this comment.
Is this what you intend the documentation to look like?
In case you do not and my code-based proposal did not work for you (which would look like this)...
... a table could also be an option:
| * @details polynom=0x1021 | |
| * seed=0x1d0f | |
| * check=0xe5cc | |
| * | |
| * Parameter | Value | |
| * --------: | :---- | |
| * Polynom | `0x1021` | |
| * Seed | `0x1d0f` | |
| * Check | `0xe5cc` |
There was a problem hiding this comment.
[..] and my code-based proposal did not work for you (which would look like this)...
You proposal was great, I just forgot about it - should not have pushed last minute before heading home...
I like the table idea - I picked it up and added some other values as well. Without going to much into detail: These parameter specify / identify one particular CRC algorithm, no more naming confusion! I initially omitted them since the check is already enough but the tables were just asking for more data. 😀
Thanks for providing your insights, I valued it a lot! :)
Teufelchen1
force-pushed
the
feat/crc_fixup
branch
from
d762999 to
d0005fa
Compare
miri64
added
CI: run tests
|
Please fix the issues pointed out by the static-tests. Other than that, I think this is ready for ACK now. |
Teufelchen1
force-pushed
the
feat/crc_fixup
branch
from
d0005fa to
a25934c
Compare
MrKevinWeiss
removed
CI: ready for build
|
Kicking this out of the CI queue until master is fixed (or blacklisted) |
MrKevinWeiss
added
CI: run tests
|
Rerunning now that master should pass... |
Moin! 👋
Contribution description
This PR adds three new crc16 variations:
The old "ccitt" was renamed to
ccitt-augas this is the "correct name" for the given implementation.Since CRCs aren't really standardised, the names are vague, many have alias. I tried to stick close to what other software projects use (linux, pypi.org/project/crccheck, etc.).
Testing procedure
I kept the original test - duplicated & renamed it accordingly.